Skip to content

Fix incorrect display of trace IDs in the automatic_logging processor#4578

Merged
ptodev merged 3 commits into
mainfrom
4552-trace-autologging-provides-incorrect-trace-ids
Jul 25, 2023
Merged

Fix incorrect display of trace IDs in the automatic_logging processor#4578
ptodev merged 3 commits into
mainfrom
4552-trace-autologging-provides-incorrect-trace-ids

Conversation

@ptodev

@ptodev ptodev commented Jul 25, 2023

Copy link
Copy Markdown
Contributor

PR Description

This is a bug which was introduced in the Otel upgrade in #3858. I must have gotten confused about the deprecation of the HexString function. Apologies for this - the bug could have been easily prevented if I'd taken more care reading the functions and the deprecation PR.

Which issue(s) this PR fixes

Fixes #4552

Notes to the Reviewer

There are two other places which use such code, but I think they are not impacted from an external user point of view:

  • The SpanToKeyVal function in pkg/integrations/v2/app_agent_receiver/payload.go appears to be unused.
  • In the service graph processor as long as a unique ID is generated (which I think is the case), then the code should work, even if the ID is not the same hex ID.

I will mention the service graph in the changelog anyway so that users are advised to upgrade.

I tested this locally for the automatic_logging processor only.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@ptodev ptodev linked an issue Jul 25, 2023 that may be closed by this pull request
@ptodev ptodev requested review from a team as code owners July 25, 2023 08:53
@ptodev

ptodev commented Jul 25, 2023

Copy link
Copy Markdown
Contributor Author

I think it's better to use .String() instead of hex.EncodeToString for two reasons:

  • It is backwards compatible.
  • The main use case for the .String() is logging, and that is exactly what the automatic_logging processor uses it for.

For other use cases such as the one in the service graph processor, using .String() makes less sense, but since it's an older piece of code I think we could just leave it as it is.

@thampiotr thampiotr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@ptodev ptodev merged commit 26d7472 into main Jul 25, 2023
@ptodev ptodev deleted the 4552-trace-autologging-provides-incorrect-trace-ids branch July 25, 2023 12:24
ptodev added a commit that referenced this pull request Jul 25, 2023
…#4578)

* Fix incorrect spanId and traceId string conversion
ptodev added a commit that referenced this pull request Jul 25, 2023
* Fix incorrect display of trace IDs in the automatic_logging processor (#4578)

* Fix incorrect spanId and traceId string conversion

* Fix upgrade guide (#4579)
dwalker-sabiogroup pushed a commit to dwalker-sabiogroup/agent that referenced this pull request Jul 28, 2023
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
clayton-cornell pushed a commit that referenced this pull request Aug 14, 2023
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0.35.0 trace autologging provides incorrect trace IDs

3 participants